[TrimmableTypeMap] Runtime-only trimmable typemap support#11090
[TrimmableTypeMap] Runtime-only trimmable typemap support#11090simonrozsival merged 21 commits intomainfrom
Conversation
b676200 to
c19872c
Compare
Rebuild the remaining runtime-only portion of PR 11090 on top of main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d5aefaa to
3a8d509
Compare
Remove the accidental fallback chain from TryGetJniName while preserving the cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the trimmable peer path on TrimmableTypeMapTypeManager/JavaMarshalValueManager, make any legacy TypeManager lookup in trimmable mode fail fast, and tighten the proxy caches/state handling for parity with the existing activation flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop silently falling back when trimmable peer activation cannot resolve a generated JavaPeerProxy. In trimmable mode, missing proxy coverage should surface as an explicit runtime error so generator gaps are fixed at the source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collapse the trimmable managed-to-JNI lookup back to a single internal API by keeping TryGetJniNameForManagedType() and removing the duplicate TryGetJniName() entry point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce TrimmableTypeMap surface area by making purely local proxy helpers private and removing the unused TryCreatePeer() wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename GetProxyForPeer() to GetProxyForJavaObject() so the helper more clearly describes what it actually inspects and resolves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 1 issue, and the PR is not mergeable yet because the required dotnet-android checks are still pending.
⚠️ Error handling:JavaMarshalValueManager.CreatePeer()now bypasses the baseJniValueManagerguards for disposed managers and invalidJniObjectReferencevalues, so the trimmable path can throw the newNotSupportedExceptionwhere the inherited contract would returnnullor raiseObjectDisposedException(src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs:510).
👍 Nice cleanup overall: the runtime-only split is much clearer now, especially the removal of the legacy TypeManager redirect and the narrower TrimmableTypeMap surface.
Review generated by android-reviewer from review guidelines.
Preserve the inherited JniValueManager.CreatePeer() preconditions for disposed managers and invalid JNI references before the trimmable fail-fast proxy check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds runtime-side support for the trimmable typemap path in Mono.Android, shifting managed↔JNI name/type resolution and peer creation away from legacy typemap codepaths and ensuring JniRuntime is published early enough for trimmable initialization.
Changes:
- Route
JNIEnv.TypemapManagedToJavato managedTrimmableTypeMapwhenRuntimeFeature.TrimmableTypeMapis enabled. - Publish the current
JniRuntimeearlier inJNIEnvInitand initializeTrimmableTypeMappost-runtime creation. - Make legacy
TypeManager.GetJavaToManagedTypeCoreexplicitly unreachable in trimmable mode and centralize peer creation viaTrimmableTypeMap.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs | Adds caches and new logic for resolving proxies/JNI names and creating peers for Java objects in trimmable mode. |
| src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs | Switches peer creation in trimmable mode to TrimmableTypeMap.CreatePeer and fails fast when proxy coverage is missing. |
| src/Mono.Android/Java.Interop/TypeManager.cs | Throws an UnreachableException if legacy java→managed lookup is used while trimmable typemap is enabled. |
| src/Mono.Android/Android.Runtime/JNIEnvInit.cs | Sets the current JniRuntime earlier and again after constructing AndroidRuntime, before typemap initialization. |
| src/Mono.Android/Android.Runtime/JNIEnv.cs | Uses managed trimmable typemap resolution for managed→Java type mapping when enabled. |
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 1 issue, and the internal dotnet-android pipeline is also currently red due to a CS0234 compile error in JavaMarshalValueManager.cs.
- ❌ API design:
TryGetTargetType()now throws for generated typemap alias entries, so a normal Java→managed lookup on an alias such asjni/name[1]will fail before the#10788follow-up lands (src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs:81).
👍 The split between proxy resolution in TrimmableTypeMap and peer activation in JavaMarshalValueManager is a nice cleanup.
Review generated by android-reviewer from review guidelines.
| if (proxy is null) { | ||
| // Alias typemap entries (for example "jni/name[1]") are not implemented yet. | ||
| // Support for them will be added in a follow-up for https://github.com/dotnet/android/issues/10788. | ||
| throw new NotImplementedException ( |
There was a problem hiding this comment.
🤖 ❌ API design — TryGetTargetType() is part of the probe path used during Java→managed resolution, but this branch now throws NotImplementedException for alias slots like jni/name[1]. Since those aliases are emitted by the typemap generator, a normal lookup will now crash before the follow-up PR lands. Can we keep alias entries non-fatal here (for example, return false until #10788 is implemented) instead of throwing?
Rule: API design
There was a problem hiding this comment.
Thanks — this is intentional for now. We are still in the middle of the trimmable typemap rollout, and alias handling is explicitly being deferred to the follow-up in #11103. I would prefer to keep that work out of this PR rather than expand the scope further.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JNIEnv.TypemapManagedToJavauses managed JNI-name resolution fromTrimmableTypeMapfor the trimmable pathJNIEnvInitpublishes the currentJniRuntimebefore trimmable typemap initialization so managed JNI services and native registration work correctlyTypeManager.GetJavaToManagedTypeis now explicitly unreachable in trimmable mode; lookups should flow throughTrimmableTypeMapTypeManagerJavaMarshalValueManager+TrimmableTypeMaphandle the remaining runtime peer creation/JNI-name resolution after the split and now fail fast if required proxy coverage is missing